Skip to content

ARROW-8354: [R] Fix segfault in Table to Array conversion#6871

Closed
fsaintjacques wants to merge 3 commits into
apache:masterfrom
fsaintjacques:ARROW-8354
Closed

ARROW-8354: [R] Fix segfault in Table to Array conversion#6871
fsaintjacques wants to merge 3 commits into
apache:masterfrom
fsaintjacques:ARROW-8354

Conversation

@fsaintjacques

@fsaintjacques fsaintjacques commented Apr 8, 2020

Copy link
Copy Markdown
Contributor

The Converter::Make did not like receiving empty ArrayVector. The bug was exposed in ARROW-8216 which could return an empty selection vector due to a randomly generated fixture in test-dplyr.R

@github-actions

github-actions Bot commented Apr 8, 2020

Copy link
Copy Markdown

Comment thread r/tests/testthat/test-dplyr.R Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should not error: it should return a 0-row data.frame:

filter(mtcars,FALSE)
##  [1] mpg  cyl  disp hp   drat wt   qsec vs   am   gear carb
## <0 rows> (or 0-length row.names)

I'll pull the branch and try to special-case that.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@fsaintjacques I did this in f93f08c.

Comment thread r/src/array_to_vector.cpp Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To save us future pain, should we hunt for other places that assume >0 length vectors? grepping [0] in r/src hits a few places that look similarly suspect.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I checked them all and found one bug, I'll add a test and the fix in the current PR.

Comment thread r/src/array_to_vector.cpp Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should accept the type of arrays explicitly; a Converter should not fail when arrays is empty. Instead the result of conversion should also be empty.

@fsaintjacques fsaintjacques Apr 8, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I updated https://issues.apache.org/jira/browse/ARROW-7798 to include this. I tried to quickly refactor what you suggest but it bubbles to rewriting almost all methods/functions of the file.

@fsaintjacques fsaintjacques Apr 8, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It also lead me to find a bug with handling Dictionary ChunkedArray, https://issues.apache.org/jira/browse/ARROW-8374

Comment thread r/tests/testthat/helper-arrow.R Outdated
@nealrichardson

Copy link
Copy Markdown
Member

#6878 may help give more visibility into the unexpected failures

fsaintjacques and others added 3 commits April 8, 2020 15:29
The Converter::Make did not like receiving empty ArrayVector. The bug
was introduced in ARROW-8216 which could return an empty selection
vector due to a randomly generated fixture in test-dplyr.R
@wesm

wesm commented Apr 8, 2020

Copy link
Copy Markdown
Member

Hmm, continuing to have arrow::r::Converter crash on no chunks isn't too great. It seems that in most cases the first element is being used to obtain type metadata. This could be fixed in most cases from what I can see by passing the type as a parameter to Converter::Make. Thoughts?

@nealrichardson

Copy link
Copy Markdown
Member

At least now it errors instead of segfaults, and I changed some of the choreography to keep away from the error. Agreed that we can do better, and I think what you say has been added to the scope of https://issues.apache.org/jira/browse/ARROW-7798.

I'm going to merge this since we've at least fixed the issue and have a passing build.

@wesm

wesm commented Apr 8, 2020

Copy link
Copy Markdown
Member

👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants